Redirect cf-url-rewriter to fetch respective index.json file #378
Conversation
Divyaasm
requested review from
peterzhuamazon,
bbarani,
gaiksaya,
rishabh6788,
zelinh,
jordarlu and
prudhvigodithi
as code owners
|
Question: Have you verified downloads with existing/older artifacts? |
Yes Rishabh, this change is compatible new folder structure |
Have a similar question as @rishabh6788 What about the |
The index file folder is updated in the corresponding build jenkins workflow when triggered. Since we do not touch the older versions they remain the same. And coming to the latest running jobs the path for index.json is present in index folder in s3 bucket. |
| @@ -66,3 +73,12 @@ function errorResponse() { | |||
| statusDescription: 'Not found', | |||
| }; | |||
| } | |||
|
|
|||
| function getIndexFilePath(versionNumber: string, urisplit: string[]): string { | |||
| if (versionNumber <= "2111") { | |||
There was a problem hiding this comment.
Based on the release history https://github.com/opensearch-project/opensearch-build/releases , I see that we are still releasing 1.3.x If we have a 1.3.15 release, looks like the logic here will still use the old index.json path.
There was a problem hiding this comment.
Just updated the requested changes. Can you please review again @tianleh. Thanks
| const urisplit = request.uri.split("/latest"); | ||
| const index = urisplit[0].lastIndexOf("/"); | ||
| const versionNumber = urisplit[0].substring(index + 1).replace(/\./g, ''); |
There was a problem hiding this comment.
nonblocking: all these variables are used for getIndexFilePath and not in the main function. Better to move them into the sub function getIndexFilePath
| (major == 1 && minor == 3 && patch >= 15) || | ||
| (major == 2 && minor >= 12) || | ||
| (major >= 3) |
There was a problem hiding this comment.
It is indeed a tricky problem. The latest patch release for 2.11 is 2.11.1 I think it is theoretically possible to have a newer patch release, say 2.11.2. Then this logic will fail. The same thing will happen to all existing 2.x releases < 2.12
So we have several options here.
Option 1: extend your current comparison logic to cover all 2.x versions. This will be a long list.
Option 2: back fill S3 buckets for all released versions with the new structure. Then you can remove the comparison. This can simplify the code but require tedious manual effort.
Option 3: always run httpsGet against the new url. If returned empty, then fall back to the old url. Then you do not need to backfill S3 buckets and also do not need to maintain a long comparison list.
I would prefer Option 3 for simplicity.
There was a problem hiding this comment.
If you want to get the semver try using https://www.npmjs.com/package/semver library.
| const data: any = await httpsGet('https://' + request.headers.host[0].value + indexUri); | ||
| const latestIndexUri = request.uri.replace(/\/latest\/.*/, '/index' + latestPath + '/index.json'); | ||
|
|
||
| try{ |
There was a problem hiding this comment.
missing indentation whitespace
| try{ | ||
| const oldIndexUri = request.uri.replace(/\/latest\/.*/, '/index.json'); | ||
| const indexData: any = await httpsGet('https://' + request.headers.host[0].value + oldIndexUri); |
There was a problem hiding this comment.
missing indentation. Please format the code
| const latestPath = request.uri.split("/latest")[1].split("/").slice(0, 4).join("/"); | ||
|
|
||
| try { | ||
| const data: any = await httpsGet('https://' + request.headers.host[0].value + indexUri); | ||
| const latestIndexUri = request.uri.replace(/\/latest\/.*/, '/index' + latestPath + '/index.json'); | ||
|
|
||
| try{ | ||
| const data: any = await httpsGet('https://' + request.headers.host[0].value + latestIndexUri); | ||
|
|
||
| if (data && data.latest) { | ||
| callback(null, redirectResponse(request, data.latest)); | ||
| } else { | ||
| callback(null, errorResponse()); | ||
| } | ||
| } catch (e) { | ||
| console.log(e); | ||
| callback(null, errorResponse()); | ||
| } | ||
| try{ | ||
| const oldIndexUri = request.uri.replace(/\/latest\/.*/, '/index.json'); | ||
| const indexData: any = await httpsGet('https://' + request.headers.host[0].value + oldIndexUri); | ||
|
|
||
| if (indexData && indexData.latest) { | ||
| callback(null, redirectResponse(request, indexData.latest)); | ||
| } else { | ||
| callback(null, errorResponse()); | ||
| } | ||
| } catch (e) { | ||
| console.log(e); | ||
| callback(null, errorResponse()); | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
You can simplify the code by extracting common logic to a sub function. Below is the skeleton.
function getLatest(url) {
try {
const data: any = await httpsGet(url);
if (data && data.latest) {
return data.latest;
}
return "";
} catch (e) {
console.log(e);
return "";
}
}
function handler(() {
...
const latestFromNewUrl = getLatest(newUrl);
if (latestFromNewUrl) {
callback(null, redirectResponse(request, latest));
} else {
const latestFromOldUrl = getLatest(oldUrl);
if (latestFromOldUrl) {
callback(null, redirectResponse(request, latestFromOldUrl));
} else {
callback(null, errorResponse());
}
}
...
}
12ac168 to
c890e0a
Compare
|
@Divyaasm Do we need this now? |
This will be used for deploying the the cf-url changes using opensearch-ci repo. I need to make the PR similar to the internal changes. We can have the test cases too here. |
|
LGTM! Please fix the merge conflict. |
Divyaasm
force-pushed
the
updateurl
branch
6 times, most recently
from
24e6c3c to
0552a0a
Compare
Signed-off-by: Divya Madala <[email protected]> Update cf-url-rewriter and add testfiles Signed-off-by: Divya Madala <[email protected]> add package.json file Signed-off-by: Divya Madala <[email protected]> Add changes Signed-off-by: Divya Madala <[email protected]> Add changes Signed-off-by: Divya Madala <[email protected]> Refactor code Signed-off-by: Divya Madala <[email protected]> Add cdn changes to support latest changes Signed-off-by: Divya Madala <[email protected]> Resolve merge conflicts Signed-off-by: Divya Madala <[email protected]>
Signed-off-by: Divya Madala <[email protected]>
Signed-off-by: Divya Madala <[email protected]>
Description
Redirect cf-url-rewriter to fetch latest buildnumber from the index.json file corresponding to the given distribution type in the url
Issues Resolved
opensearch-project/opensearch-build#4166
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.